Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synthesizer Waveforms #602

Merged
merged 33 commits into from
Oct 1, 2024
Merged

Synthesizer Waveforms #602

merged 33 commits into from
Oct 1, 2024

Conversation

iluvcapra
Copy link
Contributor

@iluvcapra iluvcapra commented Aug 10, 2024

Adds a new Source, Synth, that can generate square, triangle, sawtooth and sine waveforms useful for additive music sound synthesis. Waveforms can be generated with arbitrary sample rates and frequencies.

Also the existing SineWave Source has been reimplemented as a client of Synth.

@dvdsk dvdsk self-requested a review August 10, 2024 16:42
@iluvcapra
Copy link
Contributor Author

Can we hold on this for a moment? I'd like to add white and pink noise and I don't think it'd be too difficult.

@iluvcapra
Copy link
Contributor Author

Okay check that out.

src/source/noise.rs Outdated Show resolved Hide resolved
This resolves a PR code review note and should make the source run
much faster.
@iluvcapra
Copy link
Contributor Author

iluvcapra commented Aug 13, 2024 via email

@dvdsk
Copy link
Collaborator

dvdsk commented Aug 13, 2024 via email

@iluvcapra
Copy link
Contributor Author

iluvcapra commented Aug 14, 2024

What I'd propose is just renaming Synth into TestSignal.

I definitely think there should at least be a WhiteNoise and PinkNoise, those have a lot of applications as test signals, it's hard to subjectively judge loudness differences with sine waves, they're better for testing dynamic processes, they're obligatory for testing monitors etc. I added it because I was going to do some dynamic gain sources and it just makes more sense to test and do an example with pink noise for those.

Square and triangle waves I'm more equivocal on. A lot of signal generator plugins make them, and not in a particularly musical context. It's definitely useful to be able to see them on a scope, square waves can tell you about slew rate in a DA, triangle waves have a known spectrum and you can see aberrant harmonics easily. It's useful that these signals generate a clear known spectrum with a countable number of overtones.

If we're talking about test signals maybe I'm missing one, we could definitely use a chirp signal generator, that's really useful for evaluating frequency response.

@dvdsk
Copy link
Collaborator

dvdsk commented Aug 14, 2024

you clearly know what you are talking about which is very nice since I do not 😅. I happily defer to your judgement 👍.

And I agree on the Synth -> TestSignal rename, that is clearer. I see you have also already reimplemented the Sine source as Synth/TestSignal 👍 .

We could deprecate it in the future (the Sine source), that would clean up the API a bit.

@dvdsk
Copy link
Collaborator

dvdsk commented Aug 14, 2024

And I agree on the Synth -> TestSignal rename, that is clearer. I see you have also already reimplemented the Sine source as Synth/TestSignal 👍 .

Actually function generator or waveform generator might fit better, what do you think? Could be something like FuncGen or WaveformGen. Maybe leave off the Gen part just Function or Waveform.

Copy link
Collaborator

@dvdsk dvdsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left my opinion on the code here and there its almost exclusively about code style. Let me know if you disagree with anything, then we can discuss that in more detail.

and again thanks for the solid work!

src/source/noise.rs Outdated Show resolved Hide resolved
src/source/noise.rs Outdated Show resolved Hide resolved
src/source/noise.rs Outdated Show resolved Hide resolved
src/source/sine.rs Outdated Show resolved Hide resolved
src/source/sine.rs Outdated Show resolved Hide resolved
src/source/test_waveform.rs Outdated Show resolved Hide resolved
src/source/test_waveform.rs Outdated Show resolved Hide resolved
src/source/test_waveform.rs Outdated Show resolved Hide resolved
src/source/test_waveform.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@iluvcapra
Copy link
Contributor Author

I'll get on these in a few days, I'm finishing a Chirp source to complete the suite.

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@dvdsk dvdsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed the todo!(). So I take it you are not done yet. Take your time! its looking good!

@iluvcapra
Copy link
Contributor Author

Oh that shouldn't be there, I thought I'd deleted that, I'm not going to support try_seek with any of the signal generators based on our previous convo.

@iluvcapra
Copy link
Contributor Author

Why isn't this building in nightly BTW? I've merged the upstream so I'm not sure what the problem is.

@dvdsk
Copy link
Collaborator

dvdsk commented Sep 16, 2024

Why isn't this building in nightly BTW? I've merged the upstream so I'm not sure what the problem is.

Its building fine its just that the test builds are failing as there are undocumented methods. The wierd thing is, that lint never fired in the past... My guess is that the lint was broken and has been fixed in the latest nightly.

@dvdsk
Copy link
Collaborator

dvdsk commented Sep 16, 2024

I'll disable the lint or fix the docs

@dvdsk
Copy link
Collaborator

dvdsk commented Sep 20, 2024

if you merge main all the lint errors not related to this PR should disappear 👍

@iluvcapra
Copy link
Contributor Author

OMG this thread is so long, I'm so so sorry.

Copy link
Contributor Author

@iluvcapra iluvcapra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good for me.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
examples/signal_generator.rs Outdated Show resolved Hide resolved
src/source/test_waveform.rs Outdated Show resolved Hide resolved
src/source/test_waveform.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@dvdsk dvdsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a great PR, and brings a lot of functionality to rodio. Thank you very much for adding all this 👍

I still have some small things, if you disagree that is fine too! Then we can merge it as is.

Have a great day!

@iluvcapra iluvcapra requested a review from dvdsk October 1, 2024 05:06
@dvdsk
Copy link
Collaborator

dvdsk commented Oct 1, 2024

LGTM! 🥳

@dvdsk dvdsk merged commit 1d95a5c into RustAudio:master Oct 1, 2024
11 checks passed
@iluvcapra
Copy link
Contributor Author

Epic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants